Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portamento plugin and poly scale properties (rebase) #28

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

swesterfeld
Copy link
Collaborator

Portamento plugin (rebased), see original pull request #27

@tim-janik
Copy link
Owner

tim-janik commented Feb 11, 2018

Hey Stefan, this looks mostly good, a bit of polishing is left though:

  • Please squash the intermediate commits that contain "bug fix" and "printf". As long as things aren't merged you can still fix up the branch history to have meaningful commits only. git rebase -i also makes it really easy to fix or squash intermediate states.

  • Why is "using std::max" required in a namespace that already does "using namespace std" ? And, in general it's better not to do "using namespace std", long term that has high potential of causing conflicts, e.g. with the introduction of std::any we're likely running into problems with Aida::Any. Instead, use std::max, std::vector, or "using std::vector;" because you know that'd the one vector you mean to make use of in your code. I.e. just remove and fixup the "using namespace std" in portamento.cc.

  • Get rid of "FIXME"s in code meant to be merged into mainline. For the step/page increments I'd say just remove them in a commit and then rebase+fix that commit into the one that used to introduce the FIXMEs. If we find out later the increments don't do what we want, we can adjust them later.

  • Seeing that your plugin does two multiplications per sample just to convert back and force between frequency values makes me wonder if we choose the right path there. This should be tracked in a different bug, but what's your take on making BSE_SIGNAL_TO_FREQ / BSE_SIGNAL_FROM_FREQ NOPs, i.e. *1.0 ? DO we really rely on freqs being scaled down into 0..1 somewhere? It's not like freqs are all that useful to run through a low-pass or an amp...

  • Finally, please rebase again, to track latest master ;-)

@swesterfeld swesterfeld force-pushed the portamento branch 6 times, most recently from 449ad3a to 200e2c3 Compare March 7, 2018 20:47
@swesterfeld
Copy link
Collaborator Author

Ok, I believe I addressed all your concerns now.

Please squash the intermediate commits that contain "bug fix" and "printf". As long as things aren't merged you can still fix up the branch history to have meaningful commits only. git rebase -i also makes it really easy to fix or squash intermediate states.

Done.

Why is "using std::max" required in a namespace that already does "using namespace std" ? And, in general it's better not to do "using namespace std", long term that has high potential of causing conflicts, e.g. with the introduction of std::any we're likely running into problems with Aida::Any. Instead, use std::max, std::vector, or "using std::vector;" because you know that'd the one vector you mean to make use of in your code. I.e. just remove and fixup the "using namespace std" in portamento.cc.

Right. Today, I also believe that we should never have code like 'using namespace std', ever. I removed it and it still compiles.

Get rid of "FIXME"s in code meant to be merged into mainline. For the step/page increments I'd say just remove them in a commit and then rebase+fix that commit into the one that used to introduce the FIXMEs. If we find out later the increments don't do what we want, we can adjust them later.

Done.

Seeing that your plugin does two multiplications per sample just to convert back and force between frequency values makes me wonder if we choose the right path there. This should be tracked in a different bug, but what's your take on making BSE_SIGNAL_TO_FREQ / BSE_SIGNAL_FROM_FREQ NOPs, i.e. *1.0 ? DO we really rely on freqs being scaled down into 0..1 somewhere? It's not like freqs are all that useful to run through a low-pass or an amp...

Yes, we discussed this in person: I don't have strong feelings for either version, but I believe we could safely use plain frequencies and maybe introduce some kind of type-tag on streams to show whether a frequency is expected in the ui.

On the other hand, if speed is all you care about, as long as BSE_SIGNAL_TO_FREQ / BSE_SIGNAL_FROM_FREQ is a linear function (not exponential or anything), we could simply operate on the scaled values directly, without calling either macro once-per-sample.

Finally, please rebase again, to track latest master ;-)

Done.

Note: I also added a minimal portamento demo, which just shows off this feature in isolation. As things should be self contained, I neither used bleposc nor drum samples (i.e. from some hydrogen kit), although this would probably improve the overall sound quality of the demo.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
The scale has one parameter exponent, and should use: x^exponent

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
…lues

Due to poly scale, users can select a value of 0 or very close to 0 for
portamento time.  This change avoid problems for these cases.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld
Copy link
Collaborator Author

I rebased this branch, you should be able to merge this now without problems.

@tim-janik tim-janik added the 📃 Code PR, patch or code needs merging label Sep 28, 2018
@tim-janik tim-janik self-assigned this Oct 13, 2018
@tim-janik tim-janik added this to Blocked by Synth Engine in Blocked Issues Apr 29, 2019
@tim-janik tim-janik added the ⛔ Blocked Issues being blocked, see the project: Blocked Issues label Apr 29, 2019
@tim-janik tim-janik removed their assignment Apr 29, 2019
@Reaper10 Reaper10 mentioned this pull request Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛔ Blocked Issues being blocked, see the project: Blocked Issues 📃 Code PR, patch or code needs merging
Projects
No open projects
Blocked Issues
Blocked by Synth Engine
Development

Successfully merging this pull request may close these issues.

None yet

2 participants